-
Notifications
You must be signed in to change notification settings - Fork 609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow user to select an explicit currency for dividends #1915
base: master
Are you sure you want to change the base?
Allow user to select an explicit currency for dividends #1915
Conversation
31fd967
to
a9d382e
Compare
The custom drop-down menu doesn’t fit on my screen, and there is no way to scroll. If it is to stay at this place, it should be an actual drop-down (showing only the ISO codes), like when changing the currency of an account. But I would prefer a separate line with a label like “payment currency” or “dividend currency”. This would also provide enough space for a full currency drop-down, like in the dialogue for editing a security. Another remark: There are lots of lines with dangling spaces or tabs at the end. |
Well it's the same dropdown as under statement of assets. I don't particularly like it either (see #1876 ) but I still think it's better then the currency selection dropdown style IMO because at least it has the option of seperators for previously used currencies. I guess we could extend the other dropdowns with that too though. If other's disagree I can change it though, I'm not stuck on it. There is also this currency selection: I kinda think we should unify currency selection though, create a reusable component that looks the same everywhere and is easier to use. Try switching from EUR to USD quickly in the dropdown under accounts for example, it's a usability nightmare. Don't think this should be part of this though.
I can put it on a new line, just thought it would waste space. Do others agree?
Yeah good point, will clean it up with next commit. |
I would also propose to separate the two things:
I propose to make this PR about the first topic. Allowing other currencies makes sense. Particularly because the currency determines both: the currency of the security prices downloaded and the currency of the dividend statements. But one wants to be able to separates this: trade a security on a EUR exchange and still receive USD dividends. The change of the UI, however, is only one part. I do not know for sure, but so far in many calculations I assume that the currency fits to the currency of the security. To merge the change, I would like to see a test case (similar to what you find in the n.a.p.tests/src/scenarios folder) that tests ClientSnapshot, ClientPerformanceSnapshot, and SecurityPerformanceSnapshot procure the results you expect.
I think most of the time users will not touch the currency. I kind of like it right next to the "per share" item. |
Oh, indeed, that one is unusable in the same way. :-) (But I never use it, so I never noticed.) I‘ve just created issue #1920 for that.
As an aside, I own some shares that used to pay dividends in DEM once upon a time and now pay in EUR. So that is a use case, too. |
Ok good, I will add tests and see if things break
Ok, let's leave it where it is then |
Hey everyone, i was wondering it this PR is going to be worked on further or merged :) Maybe one thing to consider would be adding |
I do still want this feature, very much so in fact, nearly all my dividends come from US but paid in EUR. If I remember correctly I got a bit stuck trying to setup a test context that would be able to exercise all the new cases. Then I unfortunately got sidetracked and busy. From the manual testing I had done it did seem to work though. So, perhaps someone can take this as a starting point, or inspiration and finish it. If not I may pick it up sometime over the summer, but I just can't promise when. I can close the PR if you prefer. |
i wanted to pick this up but having a really rough time with eclipse (intellij guy here :D) ... it keeps crashing on me, im not sure i will be able to get at it properly, it drives me crazy |
Is the project built using Eclipse RCP? Haven't done any Java in ages, but I remember this was the last reason I had to use Eclipse for anything. :D |
I was curious so I copied the branch from chrisaut's repo and rebased it on buchen's master. There was one tiny merge conflict that was easy to resolve. I am not sure that I have a use case for this as I prefer to import my transactions from the bank's PDFs. If someone wants to pick this up but is worried about rebasing over so many commits then you could take my branch. https://github.com/ah4c/portfolio/tree/feat/explicitDividendCurrency
|
a9d382e
to
262be4c
Compare
Allows the user to select a different currency for dividend payments, different from either security currency and/or account currency.
Example use case:
US stocks bought (and tracked) in EUR at a German exchange, but Dividend gets paid in USD.
I'm not sure if this is done yet, putting it up in the hope for a quick review if the approach is acceptable.
Some Forum discussion